Skip to content

Adding color helper functions and better validation on style registration#673

Merged
kyasbal merged 1 commit into
GoogleCloudPlatform:epic/file-schema-v6from
kyasbal:push-rvnxysqvppvn
May 21, 2026
Merged

Adding color helper functions and better validation on style registration#673
kyasbal merged 1 commit into
GoogleCloudPlatform:epic/file-schema-v6from
kyasbal:push-rvnxysqvppvn

Conversation

@kyasbal

@kyasbal kyasbal commented May 21, 2026

Copy link
Copy Markdown
Member

The new file format embeds style information in the file. The older file format expect its client to have associated generated style files.

This change added some validation logics on these styles:

  • The style related registrations must be happened during task registration steps.
  • Color components must be [0-1].

@kyasbal kyasbal added this to the KHI file format v6 milestone May 21, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a locking mechanism for the timeline style registry and adds color validation to ensure RGBA values are within the [0, 1] range. It also provides utility functions for sRGB hex conversion and renames registration functions to MustRegister... to reflect their panicking behavior. Feedback focuses on correcting a typo and casing in MustForceConvertsRGBHex, adding explicit validation for the # prefix in hex strings, and maintaining naming consistency in tests. The reviewer also highlighted that renaming registration functions constitutes a breaking change.

Comment thread pkg/model/khifile/v6/style/colorutil.go Outdated
Comment thread pkg/model/khifile/v6/style/colorutil_test.go Outdated
Comment thread pkg/model/khifile/v6/style/colorutil_test.go Outdated
Comment thread pkg/model/khifile/v6/style/timeline.go
@kyasbal kyasbal force-pushed the push-rvnxysqvppvn branch from 8df7c43 to a19763b Compare May 21, 2026 00:39
@kyasbal kyasbal marked this pull request as ready for review May 21, 2026 00:40

@renamoo renamoo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we cares about it so just leaving a note that I noticed some error message starts with capital letters and some does not.

e.g.)
t.Errorf("MustForceConvertSRGBHex() mismatch (-want +got):\n%s", diff) <- start with capital
t.Errorf("expected panic, but did not panic") <- does not start with capital

@kyasbal kyasbal force-pushed the push-rvnxysqvppvn branch from a19763b to 9f400fc Compare May 21, 2026 06:36
@kyasbal

kyasbal commented May 21, 2026

Copy link
Copy Markdown
Member Author

I'm not sure if we cares about it so just leaving a note that I noticed some error message starts with capital letters and some does not.

e.g.) t.Errorf("MustForceConvertSRGBHex() mismatch (-want +got):\n%s", diff) <- start with capital t.Errorf("expected panic, but did not panic") <- does not start with capital

Error messages used in Go shouldn't be capitalized especially if it is used as the error type.
But I don't know if test assertions also shouldn't capitalized or so. However, it would be better to use lower case when the first character is a type name or method names.
I updated go coding rule for agent to increase this consistency in the later commits.

@kyasbal kyasbal force-pushed the push-rvnxysqvppvn branch from 9f400fc to 1f0a610 Compare May 21, 2026 06:44
@kyasbal kyasbal merged commit b295be3 into GoogleCloudPlatform:epic/file-schema-v6 May 21, 2026
9 of 10 checks passed
@kyasbal kyasbal deleted the push-rvnxysqvppvn branch May 21, 2026 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants